[FrameworkBundle][Routing] Add a new tag to be able to use a private service as a service route loader #30926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fancyweb
force-pushed
the
service-router-loader-private-service-
branch
3 times, most recently
from
e05abac
to
d3a2998
Compare
fancyweb
force-pushed
the
service-router-loader-private-service-
branch
from
d3a2998
to
93584eb
Compare
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderContainer.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderContainer.php
Outdated
Show resolved
Hide resolved
|
@nicolas-grekas I added the UPGRADE entries already. For the CHANGELOG of the Routing component there is nothing to say IMO. We added a marker interface, an instantly deprecated / internal class, and a compiler pass used by the Framework Bundle 😕 |
OskarStark
reviewed
Apr 7, 2019
src/Symfony/Bundle/FrameworkBundle/Tests/Kernel/MicroKernelTraitTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderContainer.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderInterface.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/DependencyInjection/ServiceRouterLoaderPass.php
Outdated
Show resolved
Hide resolved
...fony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderContainerTest.php
Outdated
Show resolved
Hide resolved
|
Ping @fancyweb. Will you have some time to make the last requested changes? It'd be nice to have this for Symfony 4.3. Thanks! |
|
I will definitely finish this work before the end of the month. |
fancyweb
force-pushed
the
service-router-loader-private-service-
branch
2 times, most recently
from
3a30564
to
9c5e31a
Compare
fancyweb
commented
Apr 29, 2019
| <service id="routing.loader.service" class="Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader"> | ||
| <tag name="routing.loader" /> | ||
| <argument type="service" id="service_container" /> | ||
| <argument type="service" id="routing.loader.service.container" /> |
There was a problem hiding this comment.
In 5.0, replace this argument with the above tagged locator.
chalasr
requested changes
May 5, 2019
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
|
Although I find |
fancyweb
force-pushed
the
service-router-loader-private-service-
branch
from
9c5e31a
to
0bb19b7
Compare
chalasr
approved these changes
May 5, 2019
fabpot
reviewed
May 6, 2019
UPGRADE-4.3.md
Outdated
| @@ -123,6 +123,7 @@ Routing | |||
| options have been deprecated. | |||
| * Implementing `Serializable` for `Route` and `CompiledRoute` is deprecated; if you serialize them, please | |||
| ensure your unserialization logic can recover from a failure related to an updated serialization format | |||
| * Not tagging the route loader services with `routing.router_loader` has been deprecated. | |||
There was a problem hiding this comment.
I think router loader is way better.
There was a problem hiding this comment.
In this case, if the "public" name is now router loader, I guess we should update the whole documentation page (https://symfony.com/doc/current/routing/custom_route_loader.html)
There was a problem hiding this comment.
Do you mean you want to move back to the original tag name routing.route_loader?
There was a problem hiding this comment.
Works for me, let's rename the interface + existing implementation, I suggest deprecating ServiceRouterLoader in favor of ContainerRouteLoader.
There was a problem hiding this comment.
WDYT of renaming everything in another PR after this one is merged for clarity ? First, finish this one (I just need to rename the tag back to routing.route_loader). Then, another one of pure refacto.
…service as a service route loader
fancyweb
force-pushed
the
service-router-loader-private-service-
branch
from
0bb19b7
to
205497b
Compare
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderContainer.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoaderInterface.php
Show resolved
Hide resolved
| * | ||
| * @deprecated since Symfony 4.3, to be removed in 5.0 | ||
| */ | ||
| class ServiceRouterLoaderContainer implements ContainerInterface |
There was a problem hiding this comment.
I suggested ContainerRouteLoader, consistent with e.g. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php
fabpot
added a commit
that referenced
this pull request
May 13, 2019
…gged locators (nicolas-grekas) This PR was merged into the 4.3 branch. Discussion ---------- [DI] default to service id - *not* FQCN - when building tagged locators | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | - | License | MIT | Doc PR | - While reviewing #30926 I realized that defaulting to the FQCN is a shortcut that isn't useful enough. Defaulting to the service id provides the same experience in practice because service ids are FQCN by default. But when they aren't, the service id is the proper index to default to when building the locator. Commits ------- 52e827c [DI] default to service id - *not* FQCN - when building tagged locators
|
#31463 is now merged, this can be rebased to leverage it and take comments into account. Thanks :) |
|
I'm closing here becaus it's a mess and I will reopen new splitted PRs 👀 |
nicolas-grekas
added a commit
that referenced
this pull request
Jul 23, 2019
…eLoader in favor of ContainerLoader and ObjectLoader (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #30926 (comment) | License | MIT | Doc PR | - This PR aims at deprecating some things to have a more consistent code. ### ServiceRouterLoader 1. This class actually fetches an object from a container. In #30926 (comment), it was suggested that it should be renamed to `ContainerRouteLoader`. Actually I think it's better to rename it to `ContainerLoader` since all others route loaders does not have "Route" in their name. Let's be consistent! 2. This class is in a `DependencyInjection` sub directory for historical reasons. Let's remove that! It accepts any PSR-11 container. ### ObjectRouteLoader 1. This class has "Route" in its name too. Let's rename it! 2. This class is supposed to be an abstract implementation for "object" loaders to reuse, but in its code it has a lot of references to "services". Let's remove those references! That means renaming some methods, altering messages, etc.. That also means removing the `supports` method from it to let extending classes implement it. 3. IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in the `ContainerLoader` class. Commits ------- 1548101 [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader
symfony-splitter
pushed a commit
to symfony/framework-bundle
that referenced
this pull request
Jul 23, 2019
…eLoader in favor of ContainerLoader and ObjectLoader (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony/symfony#30926 (comment) | License | MIT | Doc PR | - This PR aims at deprecating some things to have a more consistent code. ### ServiceRouterLoader 1. This class actually fetches an object from a container. In symfony/symfony#30926 (comment), it was suggested that it should be renamed to `ContainerRouteLoader`. Actually I think it's better to rename it to `ContainerLoader` since all others route loaders does not have "Route" in their name. Let's be consistent! 2. This class is in a `DependencyInjection` sub directory for historical reasons. Let's remove that! It accepts any PSR-11 container. ### ObjectRouteLoader 1. This class has "Route" in its name too. Let's rename it! 2. This class is supposed to be an abstract implementation for "object" loaders to reuse, but in its code it has a lot of references to "services". Let's remove those references! That means renaming some methods, altering messages, etc.. That also means removing the `supports` method from it to let extending classes implement it. 3. IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in the `ContainerLoader` class. Commits ------- 154810119d [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader
fabpot
added a commit
that referenced
this pull request
Aug 9, 2019
…rs (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [FrameworkBundle][Routing] Private service route loaders | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #30402 | License | MIT | Doc PR | symfony/symfony-docs#11337 Continuation of #30926. ~Please review only the 2nd commit, I'm building this on top of #32582 Commits ------- 64aa2c8 [FrameworkBundle][Routing] Private service route loaders
symfony-splitter
pushed a commit
to symfony/framework-bundle
that referenced
this pull request
Aug 9, 2019
…rs (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [FrameworkBundle][Routing] Private service route loaders | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#30402 | License | MIT | Doc PR | symfony/symfony-docs#11337 Continuation of symfony/symfony#30926. ~Please review only the 2nd commit, I'm building this on top of symfony/symfony#32582 Commits ------- 64aa2c8529 [FrameworkBundle][Routing] Private service route loaders
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#eufossa
This PR adds a new tag
routing.route_loaderautoconfigured thanks to a newServiceRouterLoaderinterface to be able to use private route loader services.The deprecation layer is done through a temporary container that will be removed in 5.0.
TODO :